Breakout parameters for x.509 certificate login #4463
Conversation
nicholasjackson
force-pushed
the
nic/tls_login_parameters
branch
from
e3dd5e4
to
7ce52e3
Compare
|
Can you change |
|
Hey @jefferai I think allowed_dns_sans is more descriptive, would it make sense to name all these with the same convention so:
|
|
@jefferai sorry for the delay on the updates to this, I have been traveling and then managed to catch flu. The latest commit updates things to clearly define the additional parameters as being part of the SAN extension as discussed in previous comments. |
nicholasjackson
force-pushed
the
nic/tls_login_parameters
branch
from
0a67f32
to
8a4e004
Compare
| @@ -72,6 +73,11 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, data *fra | |||
| return nil, nil | |||
| } | |||
|
|
|||
| ttl := matched.Entry.TTL | |||
There was a problem hiding this comment.
What's the reason for this and the other TTL bits below? AFAICT this is readding old code that was removed.
| @@ -324,7 +422,7 @@ func (b *backend) loadTrustedCerts(ctx context.Context, storage logical.Storage, | |||
| trustedNonCAs = make([]*ParsedCert, 0) | |||
| names, err := storage.List(ctx, "cert/") | |||
| if err != nil { | |||
| b.Logger().Error("failed to list trusted certs", "error", err) | |||
| b.Logger().Error("cert: failed to list trusted certs", "error", err) | |||
There was a problem hiding this comment.
Why are you adding this back in?
There was a problem hiding this comment.
Same with the other logging statements below.
There was a problem hiding this comment.
@jefferai I have no idea on that, I wonder if I had an old branch, pretty sure I just extended my previous pull request, might not have had changes from master applied, let me double check the Sha's
|
@nicholasjackson I removed the old stuff that had crept in. Something I can't do easily since this is on your fork instead of the main repo: Can you |
|
@jefferai 100%, I will do this first thing tomorrow morning, so this will be ready for you tomorrow |
|
Looks great! Thank you! |
This pull requests breaks out the the allowed_names parameter for restricting a certificate into three separate parameters:
The existing
allowed_namesparameter still exists however it has also been marked as deprecated in both the documentation and CLI. @jefferai I made an assumption on this if you do not intend to eventually remove theallowed_namesI can updated the docs.This pull request also includes my previous commit to add URI SAN validation, before starting this I rebased master so everything should be up to date.